feat(multiplexer): add Rust compiler cache sharing with sccache - #175

Claude finished @shepherdjerred's task in 2m 39s —— View job

PR Review: Rust Compiler Cache Sharing

This PR adds shared Docker volume caching for Rust builds. The implementation is solid overall, with good progressive enhancement design. Here are my findings:

✅ Strengths

1. Excellent progressive enhancement - Works immediately with cargo caching, improves automatically when sccache is installed
2. Good documentation - Clear comments explain what each cache does and how to enable sccache
3. Comprehensive testing - New test test_rust_caching_configured() validates all cache volumes and env vars
4. Proper test updates - All affected tests updated to account for new volume mounts (4 instead of 1)
5. User-friendly CLI - mux clean-cache with confirmation prompt is a nice touch
6. Commit message quality - Clear, explains "why", includes benefits and technical details

🔍 Issues Found

1. Security: Cache volumes mounted to /root but containers run as non-root ⚠️

Location: packages/multiplexer/src/backends/docker.rs:219

"-v".to_string(),
"mux-sccache:/root/.cache/sccache".to_string(),

Problem: The sccache volume is mounted to /root/.cache/sccache, but containers run as non-root user (line 198: --user {uid}). Non-root users can't write to /root/.cache/.

Impact: sccache won't work even when installed - it will fail with permission errors when trying to write to the cache directory.

Fix: Mount to a path the non-root user can access:

"mux-sccache:/workspace/.cache/sccache".to_string(),

And update the env var:

"SCCACHE_DIR=/workspace/.cache/sccache".to_string(),

2. Inconsistent cargo cache paths

Location: packages/multiplexer/src/backends/docker.rs:215-217

"mux-cargo-registry:/usr/local/cargo/registry".to_string(),
"mux-cargo-git:/usr/local/cargo/git".to_string(),

Question: Are these paths actually writable by the non-root user? Standard cargo paths are typically ~/.cargo/registry and ~/.cargo/git. If /usr/local/cargo is owned by root, these caches also won't work.

Verify: Check if /usr/local/cargo in the dotfiles image is owned by the user or has appropriate permissions. If not, consider using:

"mux-cargo-registry:/workspace/.cargo/registry".to_string(),
"mux-cargo-git:/workspace/.cargo/git".to_string(),

And set:

"-e".to_string(),
"CARGO_HOME=/workspace/.cargo".to_string(),

3. Logic: Volume cleanup doesn't check if volumes are in use

Location: packages/multiplexer/src/main.rs:281-285

let output = tokio::process::Command::new("docker")
    .args(["volume", "rm", volume])
    .output()
    .await?;

Issue: If any mux container is running (or stopped but not removed), docker volume rm will fail because the volume is still attached. The command continues with a warning, but the user experience could be better.

Suggestion: Before deleting volumes, either:

• Check for running/stopped containers and warn the user
• Use docker volume rm -f (force) - though this won't help if containers exist
• Better yet: list which sessions are using the volumes

Low priority - The current error message is probably adequate, but worth considering for UX.

🤔 Design Questions

4. Architecture: Why share sccache across all sessions?

Consideration: Sharing compilation caches across unrelated projects could theoretically lead to:

• Cache poisoning if different projects have identically-named but different modules
• Disk space issues if the cache grows unbounded

However: sccache uses content-addressed storage and includes compiler version in cache keys, so this should be safe. Still worth documenting the decision.

Question: Have you considered per-project or per-worktree caching instead? The current approach is probably better for your use case, but worth explicitly documenting why global sharing is safe.

5. Design: Cache location conflicts with HOME=/workspace

Location: Lines 206 and 219

You set HOME=/workspace (line 206), but then mount caches to /usr/local/cargo and /root/.cache. This means:

• Cargo's default $HOME/.cargo won't match /usr/local/cargo
• sccache's default $HOME/.cache/sccache won't match /root/.cache/sccache

The env vars (CARGO_HOME, SCCACHE_DIR) should override this, but it's cleaner to keep everything under the same HOME directory for consistency.

📝 Minor Observations

6. Test coverage: The new test is good, but consider adding:

   • A test that verifies the cache volumes persist across container recreations
   • Integration test that actually uses the cache (if feasible)

7. Documentation: The PR description is excellent. Consider adding this info to:

   • The multiplexer package README (if one exists)
   • Inline code comments about expected performance improvements

8. Naming: Volume names use mux- prefix consistently ✅

🎯 Action Items

Must fix before merge:

1. ✅ Fix sccache cache path permissions issue (mount to user-accessible location)
2. ✅ Verify cargo cache paths are writable by non-root user

Nice to have:
3. Consider improving clean-cache UX to detect active containers
4. Document why global cache sharing is safe
5. Add integration test for cache persistence

Summary

The implementation is well-thought-out with good progressive enhancement and testing. However, there's a critical permissions issue where the cache volumes are mounted to paths that non-root users likely can't write to (/root/.cache/sccache especially). This would prevent the feature from working as intended.

Fix the permissions issues and this will be good to merge!

Test Plan Note: I can't run the actual Rust compiler or tests, so please verify:

• Builds work without sccache (cargo caching only)
• Builds work with sccache (full caching)
• Cache volumes are actually writable by the non-root container user
• mux clean-cache successfully deletes volumes when no containers are running

CI Status: The CI pipeline failed - you may want to check the CI run to see if it's related to these changes.

Claude finished @shepherdjerred's task in 2m 15s —— View job

PR Review: Rust Compiler Cache Implementation

Reviewed the updated PR with focus on architectural fit, logic errors, security, and design issues that static analysis can't catch.

✅ Previous Issues Addressed

The critical permissions issue from the previous review has been fixed in commit fccf8d3c:

• ✅ Cache volumes now mounted to /workspace/.cargo and /workspace/.cache/sccache (user-writable)
• ✅ CARGO_HOME environment variable properly set
• ✅ All paths accessible by non-root user

🎯 Overall Assessment

Strong implementation with good progressive enhancement design. The code is well-tested and documented. However, there are a few issues worth addressing:

🔴 Critical Issues

None found. The previous permissions issue has been resolved.

🟡 Design & Architecture Concerns

1. Cache cleanup doesn't check for in-use volumes

Location: packages/multiplexer/src/main.rs:281-285

for volume in ["mux-cargo-registry", "mux-cargo-git", "mux-sccache"] {
    let output = tokio::process::Command::new("docker")
        .args(["volume", "rm", volume])
        .output()
        .await?;

Issue: If any mux container exists (running or stopped), docker volume rm will fail because volumes are still attached. The command continues with a warning, but the UX could be better.

Impact: User sees cryptic Docker errors like "volume is in use" without understanding why.

Suggestion: Add a check before deleting:

// Check if any mux containers exist
let containers = tokio::process::Command::new("docker")
    .args(["ps", "-a", "--filter", "name=^mux-", "--format", "{{.Names}}"])
    .output()
    .await?;
    
if !containers.stdout.is_empty() {
    eprintln!("Warning: Active mux containers detected. Stop and remove them first:");
    // List the containers
    println!("{}", String::from_utf8_lossy(&containers.stdout));
    return Ok(());
}

Priority: Medium - Current behavior works but confusing UX

2. Global cache sharing implications not documented

Location: packages/multiplexer/src/backends/docker.rs:77-87

Context: All mux sessions share the same cache volumes globally.

Pros:

• Maximum cache hit rate
• Works across worktrees and projects
• sccache uses content-addressed storage (safe)

Potential concerns:

• Unbounded disk space growth
• No cleanup mechanism besides manual mux clean-cache
• Different Rust versions could conflict (though sccache includes compiler version in keys)

Recommendation: Add documentation about:

• Expected disk usage (cargo registry can be several GB)
• When to use mux clean-cache
• How sccache ensures safety with content addressing

Priority: Low - Architecture is sound, just needs better documentation

3. Volume mount count hardcoded in tests

Location: packages/multiplexer/src/backends/docker.rs:1109-1114

// Count volume mounts (should have workspace + 3 cargo/sccache cache mounts)
let mount_count = args.iter().filter(|a| *a == "-v").count();
assert_eq!(
    mount_count, 4,
    "Normal git repo should have workspace + 3 cache mounts, got {mount_count} mounts"
);

Issue: Multiple tests assert specific mount counts (4 or 5). Adding new volume mounts in the future will break all these tests.

Better approach: Test for specific volumes rather than counting:

let has_workspace = args.iter().any(|a| a.contains("/workspace"));
let has_cargo_reg = args.iter().any(|a| a.contains("mux-cargo-registry"));
let has_cargo_git = args.iter().any(|a| a.contains("mux-cargo-git"));
let has_sccache = args.iter().any(|a| a.contains("mux-sccache"));

assert!(has_workspace && has_cargo_reg && has_cargo_git && has_sccache);

This approach is more maintainable and clearer about what's being tested.

Priority: Low - Tests work but are fragile

🟢 Strengths

1. Excellent progressive enhancement - Works immediately with cargo caching, improves automatically when sccache is installed
2. Comprehensive testing - New test test_rust_caching_configured() validates all aspects
3. Good documentation - Clear comments explain purpose and how to enable sccache
4. Proper error handling - Graceful degradation when sccache isn't installed
5. Security conscious - Uses content-addressed caching, runs as non-root
6. User-friendly CLI - mux clean-cache with confirmation is intuitive

📝 Commit Message Quality

Commit 1 (2f325254 - Initial feature):

• ✅ Clear subject line
• ✅ Explains "why" (performance benefits)
• ✅ Lists specific changes
• ✅ Includes quantified benefits (0s downloads, 90% faster)

Commit 2 (fccf8d3c - Fix):

• ✅ Clear problem statement
• ✅ Lists all changes
• ✅ Explains root cause (non-root user)
• ✅ References review feedback

Both commits follow best practices. Well done!

🔍 Code-Specific Observations

Volume naming consistency

✅ All volumes use mux- prefix consistently

Environment variable setup

✅ All required env vars are set: CARGO_HOME, RUSTC_WRAPPER, SCCACHE_DIR

Test coverage

✅ Tests verify:

• Volume mounts exist
• Environment variables are set
• Paths are user-accessible
• Cache cleanup command exists

🎯 Recommendations

Before merge:

1. Consider improving clean-cache UX to detect and warn about active containers (medium priority)

Future enhancements:
2. Add documentation about expected disk usage and cleanup strategy
3. Consider refactoring volume count assertions in tests to be more maintainable
4. Potentially add a mux cache-stats command to show disk usage

Summary

This is a well-implemented feature with good design, comprehensive testing, and clear documentation. The critical permissions issue from the previous review has been properly addressed.

The remaining suggestions are minor UX improvements and future-proofing. The code is ready to merge as-is, with the cleanup UX improvement being the only item worth considering before merge (though not blocking).

Approval recommendation: ✅ Approve with optional follow-up for cleanup UX improvement

Claude finished @shepherdjerred's task in 2m 21s —— View job

PR Review: Rust Compiler Cache Implementation

Reviewed the PR focusing on architectural fit, logic errors, security issues, and design decisions that linters/typecheckers can't catch.

Analysis Complete

This PR adds shared Docker volume caching for Rust builds across all mux sessions. The implementation follows a progressive enhancement pattern - it works immediately with cargo dependency caching and improves when sccache is installed in the dotfiles image.

Architectural Fit ✅

Excellent fit with existing patterns:

• Follows the established Docker backend structure in DockerBackend::build_create_args()
• Volume naming convention (mux-* prefix) is consistent with existing patterns
• Progressive enhancement aligns with the codebase philosophy (fail gracefully)
• Documentation style matches existing inline comments

Integration with the monorepo:

• This is a multiplexer package feature (Rust project), not a Bun workspace issue
• No conflicts with the Bun-based monorepo structure
• The feature is self-contained within the Docker backend

Logic & Edge Cases ✅

Strong error handling:

• Graceful degradation when sccache isn't installed (cargo shows warning but continues)
• All cache paths under /workspace (user-writable, non-root accessible) ✅
• Test coverage updated to account for new volume counts (3 tests updated)
• New dedicated test test_rust_caching_configured() validates all aspects

Potential edge case - Cache cleanup UX:

// main.rs:282-285
for volume in ["mux-cargo-registry", "mux-cargo-git", "mux-sccache"] {
    let output = tokio::process::Command::new("docker")
        .args(["volume", "rm", volume])
        .output()
        .await?;

If any mux containers exist (even stopped), docker volume rm will fail. The command handles this gracefully with a warning, but the UX could be better. Consider adding a pre-check to detect active containers:

// Check if any mux containers exist
let containers = tokio::process::Command::new("docker")
    .args(["ps", "-a", "--filter", "name=^mux-", "--format", "{{.Names}}"])
    .output()
    .await?;
    
if !containers.stdout.is_empty() {
    eprintln!("Warning: Active mux containers detected:");
    println!("{}", String::from_utf8_lossy(&containers.stdout));
    eprintln!("\nStop and remove containers before cleaning cache.");
    return Ok(());
}

Priority: Low - current behavior is acceptable, just confusing when volumes are in use.

Security ✅

Previous permissions issue resolved:

• Commit b695a7db fixed the critical issue from first review
• All caches now under /workspace (user home, not /root)
• Containers run as non-root (--user {uid} at line 199)
• Read-write access is correct for caches (they need to be writable)

Content-addressed caching safety:

• sccache uses content-addressed storage with compiler version in keys
• Safe to share globally across projects
• No risk of cache poisoning between different projects

Design Decisions ✅

Progressive enhancement is the right choice:

• Works today with cargo caching (0s dependency downloads)
• Automatically improves when sccache is added (90%+ faster compilation)
• No breaking changes, no migration needed

Global cache sharing:
The decision to share caches across ALL mux sessions (not per-project) is sound:

• ✅ sccache is content-addressed and includes compiler version
• ✅ Maximizes cache hit rate across worktrees
• ✅ Simplifies implementation (no session-specific volume names)
• ⚠️ Could grow unbounded without manual cleanup

Documentation completeness:

• Inline comments clearly explain what each cache does
• Comment at lines 77-87 explains the progressive enhancement approach
• PR description has excellent detail on benefits and how to enable sccache

Minor suggestion: Add expected disk usage to help users understand when to run mux clean-cache:

/// Shared cache volumes used across all mux Docker containers for faster Rust builds:
/// - mux-cargo-registry: Downloaded crates from crates.io (~1-5GB typical)
/// - mux-cargo-git: Git dependencies (~100MB-1GB)
/// - mux-sccache: Compilation cache (~500MB-5GB depending on projects)
///
/// Run `mux clean-cache` to free disk space if needed.

Test Quality ✅

Comprehensive test updates:

• New test test_rust_caching_configured() validates:

  • All 3 volume mounts exist
  • CARGO_HOME environment variable set
  • RUSTC_WRAPPER points to sccache
  • SCCACHE_DIR configured correctly

• Updated existing tests to expect 4 volumes instead of 1:

  • test_non_worktree_no_extra_mounts()
  • test_malformed_git_file_graceful_failure()
  • test_missing_parent_git_graceful_failure()

Test brittleness observation:
Multiple tests hardcode volume counts (lines 1111, 1226, 1261):

let mount_count = args.iter().filter(|a| *a == "-v").count();
assert_eq!(mount_count, 4, "Expected 4 mounts...");

This works but is fragile - adding new volumes in the future breaks all these tests. Consider testing for specific volumes instead:

let has_workspace = args.iter().any(|a| a.contains("/workspace"));
let has_cargo_reg = args.iter().any(|a| a.contains("mux-cargo-registry"));
// ... etc
assert!(has_workspace && has_cargo_reg && has_cargo_git && has_sccache);

Priority: Low - tests work, just future-proofing consideration

Commit Messages ✅

Commit 1: d0be867c - "feat(multiplexer): add Rust compiler cache sharing with sccache"

• ✅ Clear conventional commit format
• ✅ Explains the "why" (performance benefits)
• ✅ Lists all changes (volumes, env vars, CLI command, tests)
• ✅ Quantified benefits (0s downloads, 90% faster compilation)
• ✅ Includes instructions to enable sccache

Commit 2: b695a7db - "fix: use non-root accessible paths for Rust cache volumes"

• ✅ Clear problem statement (root paths not accessible by non-root user)
• ✅ Lists all path changes
• ✅ Explains root cause
• ✅ References review feedback

Both commits follow best practices with clear explanations of what changed and why.

Summary

This is a well-designed, well-implemented feature that fits perfectly into the existing codebase architecture. The progressive enhancement pattern is the right choice, the security concerns from the first review have been properly addressed, and the testing is comprehensive.

Recommendations

Optional improvements (not blocking):

1. Cache cleanup UX: Add container detection before attempting docker volume rm to provide better error messages when volumes are in use
2. Documentation: Add expected disk usage estimates to help users understand cache growth
3. Test maintainability: Consider testing for specific volumes rather than hardcoding mount counts

Approval Status

✅ Ready to merge

The implementation is solid, security is good, tests are comprehensive, and it follows codebase patterns. The optional improvements above are nice-to-haves that could be addressed in follow-up PRs.